Review: coder_eval — pr:97 (18 files) axis:1,2,3,4,5,6,7,8

Scope: pr:97 (18 files) axis:1,2,3,4,5,6,7,8 · branch codex/generic-agent-worker · ce92037 · 2026-08-11T04:14Z · workflow variant

Change class: complex — introduces a new cross-process nonce-framed RPC protocol, a stateful unprivileged worker owning the full Agent lifecycle, kernel-identity/capability handshakes, and fail-closed teardown; correctness requires reasoning about concurrency, security boundaries, and the Agent ABC contract

The codebase remains in strong shape (9.1/10) — type safety, API surface, security and architecture all score 9.4+, and the new root/UID-2000 agent-isolation boundary is a genuine improvement — but the risk is concentrated in the new isolation/agent_worker.py seam, whose weakest axis (Evaluation Harness Quality, 7.5) reflects three real ways a scored run can silently degrade for identical agent output: hard-killed isolated turns lose their token/cost/transcript telemetry, the privilege-drop capability label was not bumped when the host-side agent-kind allowlist was removed (so a stale-but-labeled image fails open to root), and the worker stays ptrace/memory-readable by its own same-UID descendants (so the graded trajectory and reported cost are forgeable); combined with a 28.6%-covered, deadline-free RPC layer that is now the production agent path for every containerized run, the bottom line is that the feature is architecturally sound but needs the telemetry, fail-closed and coverage gaps closed before it is trusted as the default.

Summary

Overall Score: 9.1 / 10 · Weakest Axis: Evaluation Harness Quality at 7.5 / 10
Totals: 🔴 0 · 🟠 4 · 🟡 6 · 🔵 6 across 8 axes.

Blockers

1. [Axis 3] The new isolation worker/proxy boundary has essentially no behavioral tests: proxy RPC + crash-partial handoff + teardown, _WorkerServer.handle dispatch branches, both fail-closed security gates (identity handshake, RPC nonce), and cross-process event forwarding are all uncovered (src/coder_eval/isolation/agent_worker.py:337) — agent_isolation defaults to True (src/coder_eval/models/sandbox.py:198 agent_isolation: bool = Field(default=True,), and orchestrator.py:1316-1324 now returns IsolatedAgentProxy(...) instead of create_agent(...) on that path — so this class is the production Agent for every containerized run. The only test that touches it is an identity check: tests/test_orchestrator.py:379 assert isinstance(agent, IsolatedAgentProxy) / :380 assert agent.agent_kind == "claude-code". grep -rn "IsolatedAgentProxy" tests/ returns exactly those two assertion lines plus the import. Routed coverage confirms it: agent_worker.py = 412 stmts / 278 missing / 28.60%, with 371-408 (_spawn), 411-448 (_read_stdout), 473-490 (_request), 493-504 (_apply_snapshot), 508-530 (_raise_remote_error), 539-553 (start), 564-600 (communicate), 610-623 (stop), 626-637 (kill_sync) all uncovered. The single highest-value missing case is the crash contract at lines 587-593 — except BaseException: → self.pending_turn = partial.model_copy(update={"crashed": True, "crash_reason": "agent worker terminated"}) — which is the exact crashed=True partial-TurnRecord handoff the orchestrator's _on_attempt_failure drains; if it regresses, a crashed turn silently vanishes from result.turns and the persisted task.json. Add unit tests that drive proxy↔worker in-process: extract the launch argv (CONTAINER_DROP_SHIM, sys.executable, "-I", "-m", ... at lines 375-379) into an overridable seam so a test can spawn the worker module directly (no drop shim, any platform) against a registry test agent like the existing _PluginAgent, then assert (i) a normal turn round-trips, (ii) a worker killed mid-turn yields pending_turn.crashed is True, (iii) a worker-raised TurnTimeoutError re-raises as TurnTimeoutError with timeout_seconds/iteration preserved.
2. [Axis 6] No per-RPC deadline on the worker handshake: start()/ping run before the task_timeout watchdog arms, so a live-but-mute worker hangs the task forever (src/coder_eval/isolation/agent_worker.py:484) — _request awaits the response future with no deadline — stop() is the only RPC with one (line 616, asyncio.wait_for(self._request("stop", {}), timeout=_STOP_TIMEOUT_SECONDS)):

472:    async def _request(self, method: str, params: dict[str, Any]) -> Any:
...
484:            response = await future

and the handshake plus start run unguarded:

390:        hello = await self._request("ping", {})

Failure scenario: IsolatedAgentProxy.start() is invoked from Orchestrator._setup() (orchestrator.py:1105 inside execute_with_retry at 1111, which applies retries but NO timeout — verified: zero timeout/wait_for in errors/executor.py and errors/retry.py), and _setup() is awaited at orchestrator.py:468, i.e. BEFORE with ThreadedWatchdog( at orchestrator.py:495 arms task_timeout. orchestration/batch.py has no per-task timeout (zero timeout matches) and DockerRunner puts no wall clock on docker run (its heartbeat at docker_runner.py:135-142 detects HOST death, not a stalled container). So a worker that holds stdout open without answering ping/start — e.g. the third-party AgentRegistry plugin this PR exists to support blocks inside ensure_plugins_loaded() (line 224), or the setpriv shim wedges — hangs the task, the batch, and the nightly job indefinitely. This is a regression: the replaced ClaudeCodeAgent.start() is pure local assignment (agents/claude_code_agent.py:737-741) and cannot hang. discard_pending_turn (line 606), on the crash-recovery path, is likewise unbounded.
Aggravating: _WorkerWriter.write swallows a failed response write — with self._lock, contextlib.suppress(BrokenPipeError, OSError): (line 197) — so the worker cannot report that the reply never left, leaving the untimed parent waiting on a future nothing will resolve.
Fix: give _request a per-method deadline (a startup/handshake timeout for ping/start, turn_timeout-derived for communicate) and raise AgentCrashError/TurnTimeoutError after killing the worker; do not suppress OSError on a response write — abort the worker so the parent sees EOF.
3. [Axis 8] Host-side agent-kind allowlist removed without bumping the uid-gid-v1 capability label, so a stale-but-labeled image plus new host code runs a third-party plugin agent as root with no error (src/coder_eval/isolation/docker_runner.py:701) — _validate_agent_isolation_compatibility lost its allowlist (supported_agents = {CLAUDE_CODE, CODEX, ANTIGRAVITY, NONE} and the raise DockerRunError("...has no verified UID-drop launch seam for agent type...")); line 701-702 is now just if not self._docker_config.agent_isolation: return. That gate is HOST-side; the replacement enforcement (IsolatedAgentProxy, orchestrator.py:1313-1323) lives in the IMAGE. The image capability label is unchanged — docker/Dockerfile:110 still LABEL org.coder-eval.agent-isolation="uid-gid-v1" — and the only version check, _preflight_image_version (docker_runner.py:254-259), is a logger.warning ("Image %s coder_eval %s != host %s. Rebuild with make docker-image"), i.e. soft-fail by design.

So new host code + any pre-PR uid-gid-v1 image passes _preflight_agent_isolation_image (line 288), the host no longer rejects a third-party AgentRegistry kind, and the old in-container orchestrator has no launch seam for it: it calls create_agent(...) in the root process, so the plugin agent and all candidate code run as ROOT with full access to /opt/coder-eval/grader (hidden task data, references, the run output dir). Isolation fails OPEN and silently — exactly the fail-closed property #87 was built for. The reverse skew (new image + old host code) is benign (old host rejects the kind).

Fix: bump the capability label to a new value (e.g. uid-gid-v2) in docker/Dockerfile:110 and require it in _preflight_agent_isolation_image, so a stale image is rejected loudly instead of degrading; or make _preflight_image_version a hard error when agent_isolation is on. Either way, state in the PR body that make docker-image / the ghcr push is a lockstep prerequisite — the nightly runs :latest from ghcr (docker-publish.yml pushes on main), so between merge and the next image publish the nightly is in exactly this skew window.
4. [Axis 8] Hard-killed isolated turns lose token/cost/transcript telemetry: killpg(SIGKILL) on the worker session prevents the terminal AgentEndEvent, so the recovered partial has token_usage=None and agent_output="" (src/coder_eval/isolation/agent_worker.py:588) — On the task_timeout path the watchdog calls agent.kill_sync() (orchestrator.py:493) and Orchestrator._drain_killed_turn (orchestrator.py:640-671) recovers the parked partial, logging "Recovered the hard-killed turn: %d tokens, %s" — and the docstring states the recovered turn "feeds token aggregation and command stats like any other". In-process that partial is complete: ClaudeCodeAgent catches the CancelledError and calls self._finalize_external_cancel(state.finalize) (claude_code_agent.py:1024), which emits the terminal AgentEndEvent carrying usage, messages, agent_output, duration_seconds.

IsolatedAgentProxy.kill_sync (agent_worker.py:625-637) instead killpg(process.pid, SIGKILL)s the whole worker session, so the in-worker agent's cancel handler never runs and no AgentEndEvent ever crosses the pipe. The proxy's fallback at agent_worker.py:587-593 (except BaseException: / partial = collector.build_turn_record()) therefore hits EventCollector.build_turn_record's if end is None: branch (streaming/collector.py:149-160), which returns token_usage=None, messages=[], agent_output="", duration_seconds=0.0. Net effect on every isolated run that hits run_limits.task_timeout or the wait_for backstop: the final (usually longest) turn contributes 0 tokens and "unpriced" cost to total_token_usage, has an empty transcript, and reports agent_output="" to any trajectory-consuming criterion (llm_judge) — so cost/token rows in the nightly reports under-report and a judge score can differ for identical agent output. The reconciliation invariant is not violated (no token_usage to reconcile against) but the data is simply gone.

Fix: before SIGKILL, give the worker a chance to finalize — e.g. send SIGTERM to the worker only (not the group), let _serve_worker's finally: await server.close() / the agent's cancel handler emit the terminal AgentEndEvent, then escalate to killpg(SIGKILL) after a short grace; or have the worker install a SIGTERM handler that finalizes the in-flight turn and writes one last event frame. Add a test asserting a killed isolated turn still yields a partial with non-None token_usage.

Non-blocking, but please consider before merge

1. [Axis 1] New worker RPC codec hand-mirrors the ApiRoute union, the error hierarchy and the Agent getters in literal maps, with no exhaustiveness/parity guard (bypasses tests/test_route_seam_exhaustiveness.py) (src/coder_eval/isolation/agent_worker.py:112) — _route_to_payload keys the wire format on type(route).__name__ (line 106: return {"type": type(route).__name__, "data": dataclasses.asdict(route)}) and _route_from_payload decodes it with a hand-maintained literal map:

112:    route_types: dict[str, type[DirectRoute] | type[BedrockRoute] | type[LiteLLMRoute]] = {
113:        "DirectRoute": DirectRoute,
114:        "BedrockRoute": BedrockRoute,
115:        "LiteLLMRoute": LiteLLMRoute,
116:    }

src/coder_eval/models/routing.py:126 already owns exactly this concern: ROUTE_NAMES: dict[type, str] = {...} — "Stable string names for environment_info recording (decoupled from class names)". A fourth ApiRoute variant added to the union at routing.py:122 must now be mirrored in both tables, and only one of them lives next to the union. Line 520 repeats the same shape a second time (exception_types: dict[str, type[Exception]] = {...} with 7 literal class names), so an error type not in that list silently degrades to AgentCrashError. Derive both maps from the SSOT — build the route codec off ROUTE_NAMES (or a {cls.__name__: cls for cls in typing.get_args(ApiRoute)} comprehension) and colocate the exception map with coder_eval.errors — and add a test asserting every get_args(ApiRoute) member round-trips through _route_to_payload/_route_from_payload.
2. [Axis 1] Privilege-drop/identity handshake collapses 10 conditions into one boolean and rejects with a single opaque error that dumps raw state without naming the failed field or expected value (src/coder_eval/isolation/agent_worker.py:209) — uv run radon cc -s -n C src/coder_eval/isolation/agent_worker.py at PR HEAD: _WorkerServer.handle - C (16) @209, IsolatedAgentProxy._read_stdout - C (15) @410, IsolatedAgentProxy._spawn - C (13) @370, _serve_worker - C (12) @277 — all 100% new code, in a module that is now on the production Docker run path. The worst readability cost is a single boolean at 394–405 that mixes process spawn with an 11-clause kernel assertion:

394:        privilege_drop_ok = (
395:            isinstance(hello, dict)
396:            and hello.get("uid") == AGENT_UID
...
404:            and all(value == 0 for value in capabilities.values())
405:        )
406:        if not privilege_drop_ok:
...
408:            raise RuntimeError(f"agent worker did not enter the configured unprivileged security domain: {hello!r}")

An operator debugging a failed handshake gets the whole hello dict and must diff 11 conditions by eye. Extract a _privilege_drop_mismatch(hello) -> str | None that returns the first failing check by name (and name it in the error), splitting _spawn into spawn + verify. Likewise split handle (a 5-branch method-name ladder → a {method: coroutine} dispatch table) and _read_stdout (frame parse vs. event dispatch vs. response routing) into helpers.
3. [Axis 2] Live _WorkerWriter callback smuggled through the JSON params dict under a magic "_writer" key, so it is never type-checked against StreamCallback (src/coder_eval/isolation/agent_worker.py:248) — params is the model of the JSON request body (async def handle(self, method: str, params: dict[str, Any]) -> tuple[Any, bool], line 209), but a live in-process object is injected into it and pulled back out untyped:

            if request.get("method") == "communicate":
                params["_writer"] = writer          # line 310
...
            writer = params.pop("_writer")          # line 248 -> typed Any
            record = await self.agent.communicate(
                str(params["user_input"]),
                stream_callback=writer,             # line 251

Because params.pop(...) on a dict[str, Any] yields Any, pyright performs no check that _WorkerWriter (lines 188-202) satisfies the StreamCallback Protocol (streaming/callbacks.py:12-17) — this is the only place the two are connected, and it is the one place the connection is unchecked. _WorkerWriter never declares conformance either (no StreamCallback base, no explicit annotation), so if on_event's signature changes the breakage surfaces only at runtime, inside the unprivileged worker subprocess, mid-turn. It also makes the declared type of params untrue for its actual contents (a non-JSON object inside the JSON payload dict), which is why strict pyright reports agent_worker.py:313:87 - error: Argument type is partially unknown ... "dict[Unknown, Unknown]" for the handle(..., params) call.

Fix: hand the writer over as a real parameter instead of a dict key — e.g. async def handle(self, method: str, params: dict[str, Any], *, writer: StreamCallback | None = None) — and drop the params["_writer"] injection at line 310. That restores the protocol check at the call site at zero runtime cost. Minimum viable alternative: annotate the local (writer: StreamCallback = params.pop("_writer")).
4. [Axis 4] Agent worker stays ptrace/memory-accessible to its own UID-2000 descendants (dumpable resets to 1 on exec), so the RPC nonce is recoverable from worker memory and the worker can be hijacked into emitting frames the root harness fully trusts — but the cited /proc/<worker_pid>/fd/0 read and /proc/<worker_pid>/fd/1 write route does NOT work (root-owned 0600 pipe inodes ⇒ EACCES) (src/coder_eval/isolation/agent_worker.py:416) — The whole point of this PR is a root-orchestrator / UID-2000-agent privilege boundary, and the RPC framing is the only authentication on the root-inbound side. _spawn gives the worker stdout=asyncio.subprocess.PIPE (line 381) and the root writes every request — nonce included — into the worker's stdin: request = {"nonce": self._nonce, "id": request_id, "method": method, "params": params} (line 480). After setpriv --reuid=agent, the worker execs a non-setuid python, so begin_new_exec resets dumpable to SUID_DUMP_USER and /proc/<worker_pid> is owned by 2000:2000 — the same UID as every tool subprocess the agent drives. An agent Bash command can therefore (a) read pending request bytes (containing "nonce":"<64 hex>") off /proc/<worker_pid>/fd/0 on any post-first-turn RPC (communicate in dialog mode, discard_pending_turn, stop), and (b) write forged newline-terminated frames to /proc/<worker_pid>/fd/1, which is the write end of the channel the root parses. The root then acts on those frames with full trust: it resolves the pending future (line 436-440), feeds EventCollector.on_event (line 429), runs _apply_snapshot (line 488/585) and TurnRecord.model_validate(result.get("record")) (line 586) — i.e. the agent can fabricate the scored trajectory that command_executed / skill_triggered / commands_efficiency grade against, and under-report token_usage.total_cost_usd to evade the max_usd gate. Even without the nonce, the unauthenticated fallback at line 416-418 (if not line.startswith(self._response_prefix): logger.info("[agent-worker] %s", line)) lets the agent inject arbitrary lines into the trusted task.log/log_tail, and a partial write with no trailing newline desyncs a legitimate frame so its response is silently logged instead of delivered (the pending future then hangs until the turn timeout). Concrete fix: call prctl(PR_SET_DUMPABLE, 0) in agent_worker_internal_command() before any agent is constructed — with dumpable=0 the kernel re-owns /proc/<worker_pid> as root:root and UID-2000 descendants can neither open its fds nor read its memory; additionally move the frame channel off fd 1 (a dedicated pass_fds socketpair) and compare the nonce with hmac.compare_digest instead of str.startswith. NOTE for the verifier: the base branch had an equivalent exposure (the agent could inject stream-json into the claude CLI's stdout, which the root SDK parsed), so this is a hardening gap in a NEW security control rather than a regression — hence medium, not high. CVSS:3.1/AV:L/AC:H/PR:L/UI:N/S:C/C:L/I:H/A:N (6.4)
5. [Axis 5] SAFE_CODER_EVAL_ENV allowlist is hand-picked, not consumer-derived: it exempts CODER_EVAL_IN_CONTAINER (no reader, contradicting the doc this PR edited) while the blanket CODER_EVAL prefix scrub strips CODER_EVAL_RAW_SDK_LOG, which the worker reads (src/coder_eval/isolation/agent_worker.py:52) — _SAFE_CODER_EVAL_ENV = frozenset({"CODER_EVAL_IN_CONTAINER"}) (line 52) is the sole exemption from the blanket prefix scrub at line 62 (if name in _SCRUB_ENV_VARS or (name.startswith("CODER_EVAL_") and name not in _SAFE_CODER_EVAL_ENV)). Both halves are wrong now that the whole agent lifecycle moved into the worker: (a) grep -rn IN_CONTAINER --include="*.py" src/coder_eval/ shows CODER_EVAL_IN_CONTAINER is only set (docker_runner.py:1330) and allowlisted here — no src/ reader consumes it (tests/test_codex_agent.py:108 even notes "_build_thread_options reads neither CODER_EVAL_IN_CONTAINER nor os.name"), so the allowlist entry is dead; (b) CODER_EVAL_RAW_SDK_LOG IS read agent-side, at src/coder_eval/agents/_logging.py:21 (_RAW_SDK_LOG_ENV = "CODER_EVAL_RAW_SDK_LOG"), which now executes inside the worker — so the knob CLAUDE.md documents as "Set CODER_EVAL_RAW_SDK_LOG=1 to dump every raw SDK event to the task log for inspection" silently does nothing under the new default (agent_isolation: true), exactly when a maintainer most needs it to debug a container run. Fix: derive the allowlist from real worker-side consumers — add CODER_EVAL_RAW_SDK_LOG and drop the unread CODER_EVAL_IN_CONTAINER (or keep it with a comment naming its non-Python consumer) — and add a test asserting the diagnostic knob survives build_agent_worker_environment(). This is the same dead-config class CE031 already guards for RunLimits/Dataset/SimulationConfig.
6. [Axis 8] The isolated agent worker configures no logging, so agent-side DEBUG diagnostics no longer reach task.log, and the scrubbed CODER_EVAL_RAW_SDK_LOG makes the raw-SDK dump unreachable under isolation (src/coder_eval/isolation/agent_worker.py:334) — task.log is written by an in-process logging.FileHandler(..., level=DEBUG) (logging_config.py:263-297), so before this PR every ClaudeCodeAgent/CodexAgent DEBUG/INFO record landed in it. The agent now lives in a separate process and agent_worker_internal_command (line 331-334) is just asyncio.run(_serve_worker()) — no setup_logging, no handler — so worker-side records fall to Python's lastResort handler (stderr, WARNING+). Only those WARNING+ lines survive, re-logged one level down by _read_stderr (line 461, logger.info("[agent-worker] %s", ...)). Every DEBUG/INFO diagnostic the agents emit (SDK stream progress, retry/route decisions, _log.debug calls) is now absent from task.log and therefore from the HTML report's log tail — the primary artifact for triaging a failed nightly task.

Compounding it, build_agent_worker_environment (line 62: if name in _SCRUB_ENV_VARS or (name.startswith("CODER_EVAL_") and name not in _SAFE_CODER_EVAL_ENV)) drops CODER_EVAL_RAW_SDK_LOG, which agents/_logging.py:21 reads inside the worker. The documented debug switch (CLAUDE.md: "Set CODER_EVAL_RAW_SDK_LOG=1 to dump every raw SDK event to the task log") is a silent no-op whenever isolation is on — i.e. by default on the docker driver.

Fix: call the harness logging setup at the top of agent_worker_internal_command writing to stderr at DEBUG (the proxy already forwards stderr), and add CODER_EVAL_RAW_SDK_LOG (plus CODER_EVAL_DEBUG) to _SAFE_CODER_EVAL_ENV — they are diagnostic switches, not harness paths or credentials.

Nits

1. [Axis 1] _error_snapshot hand-duplicates _snapshot's payload shape (src/coder_eval/isolation/agent_worker.py:143) — _error_snapshot's fallback re-derives the same four keys _snapshot (lines 131-140) already builds:

154:            return {
155:                "state": state.value,
156:                "pending_turn": pending.model_dump(mode="json") if pending is not None else None,
157:                "sdk_options": None,
158:                "environment": {},
159:            }

Only the last two keys differ from _snapshot, so a fifth field added to the snapshot contract silently vanishes from every error response. Collapse to one writer, e.g. have _snapshot take include_optional: bool = True (skipping the get_sdk_options() / get_environment_info() calls when False) and make _error_snapshot call it with False in its except branch.
2. [Axis 1] Agent username hardcoded as a literal twice while the exported AGENT_USERNAME constant sits unused (src/coder_eval/isolation/agent_worker.py:67) — build_agent_worker_environment imports AGENT_HOME from coder_eval.models but inlines the username next to it:

66:            "HOME": AGENT_HOME,
67:            "LOGNAME": "agent",
68:            "USER": "agent",

src/coder_eval/models/container_paths.py:30 defines AGENT_USERNAME = "agent" and models/__init__.py exports it; grep -rn AGENT_USERNAME src/ docker/ tests/ shows it has no consumers at all, so this PR added the first two sites that should have used it and used literals instead. Same pattern one line up: _SCRUB_ENV_VARS (line 53) took over the deleted utils.AGENT_ENV_SCRUB_VARS, but the companion prefix is now inlined as a bare string at line 62 (name.startswith("CODER_EVAL_")) rather than a named constant. Use AGENT_USERNAME for both env values and hoist the prefix into a module constant beside _SAFE_CODER_EVAL_ENV.
3. [Axis 1] Stream events are framed twice — the outer nonce frame wraps a complete inner wire line (src/coder_eval/isolation/agent_worker.py:202) — _WorkerWriter.on_event embeds a fully framed wire line as a JSON string inside the nonce frame — line 202: self.write({"kind": "event", "event": serialize_event(event)}) — and streaming/wire.py::serialize_event already prepends LINE_PREFIX = "\x1ecoder-eval-stream\x1e:". The host then strips it again at line 424 (deserialize_event(str(payload.get("event", "")))). Inside the outer \x1ecoder-eval-agent-rpc\x1e:<nonce>: frame the inner sentinel carries no information, and being a nested JSON string it gets escaped (�…) on every event. Split wire.py into an event_payload(event) -> dict / event_from_payload(dict) pair and embed the dict directly, keeping serialize_event/deserialize_event as the thin prefix wrappers for the stdout-line transport that actually needs them.
4. [Axis 4] Stop-flag tempdir hardcodes dir="/tmp" (bandit B108) with no nosec justification, and leaks on any path that skips communicate()'s finally (src/coder_eval/isolation/agent_worker.py:176) — Explicit disposition of the routed bandit Medium: the classic "insecure temp" reading of directory = Path(tempfile.mkdtemp(prefix="coder-eval-agent-stop-", dir="/tmp")) / directory.chmod(0o711) (lines 176-177) is a FALSE POSITIVE — mkdtemp is race-safe (mkdir with O_EXCL semantics, mode 0700, no symlink following), in-container /tmp is sticky 1777 so UID 2000 cannot unlink the root-owned dir, and 0o711 is deliberately correct for the root-writes / worker-stats handoff: the worker only needs stop_path.exists() (line 254), which requires traverse (x) but not read (r) or write (w), so the agent can neither create nor delete the stop flag. Two real residual nits: (1) dir="/tmp" is hardcoded rather than letting tempfile honour TMPDIR, and the world-traversable mode publishes the flag directory's existence to every process in the container — pass no dir= (or a root-only 0700 parent under /opt/coder-eval/) and chmod only the parent chain actually needed; (2) _remove_stop_path is only invoked from communicate's finally (line 600), so an orchestrator SIGKILL or a crash between _new_stop_path() (line 567) and entry to the try leaks the directory for the container's lifetime. Whichever way it is resolved, add # nosec B108 - root-created 0711 flag dir in a sticky container /tmp; mkdtemp is race-safe so this Medium stops being re-triaged on every bandit run (2 other findings in the tree already carry explicit nosec justifications). CVSS:3.1/AV:L/AC:H/PR:L/UI:N/S:U/C:L/I:N/A:N (2.5)
5. [Axis 5] New 663-line module carries both RPC sides plus protocol, env builder and kernel handshake, with redundant function-level imports obscuring its dependency graph (src/coder_eval/isolation/agent_worker.py:391) — agent_worker.py is one 663-line / 412-statement module holding the worker server (_WorkerServer, _serve_worker, agent_worker_internal_command), the root-side client (IsolatedAgentProxy, ~320 lines), the wire framing constants, the least-privilege env builder, and the /proc/self/status kernel handshake — radon flags four new C-grade blocks in it (_WorkerServer.handle C(16) @209, _read_stdout C(15) @410, _spawn C(13) @370, _serve_worker C(12) @277). Co-locating both protocol ends is a defensible SSOT choice, so this is a cohesion note rather than a defect; splitting the proxy from the server (with the shared frame constants in a small third module) would make each side independently readable. Concretely fixable now: line 391 from coder_eval.models import AGENT_GID, AGENT_UID duplicates the module-level from coder_eval.models import (...) already present at line 29, and line 371 from coder_eval.isolation.agent_identity import require_isolation_runtime defers an intra-package sibling import that cannot cycle — neither deferral is required by CE017, and both make the module look like it is working around an import cycle when it is not. Hoist both to module scope.
6. [Axis 7] agent_worker_internal_command follows the hidden-CLI-command naming convention but is a main hook, and the new public names are not in any all (src/coder_eval/isolation/agent_worker.py:331) — Every other *_internal_command in the tree is a hidden Typer command: src/coder_eval/cli/run_task_internal_command.py::run_task_internal_command is registered at cli/__init__.py:84 (app.command(name="_run-task-internal", hidden=True)(run_task_internal_command)). agent_worker_internal_command() is named identically but is never registered; the actual launch path is python -I -m coder_eval.isolation.agent_worker (line 379 and the Dockerfile sanity check at docker/Dockerfile:101), driven by the if __name__ == "__main__" block at line 662. Rename it to main() (or register it as a hidden command like its sibling) so the convention stays a reliable signal. Relatedly, this module adds three non-underscore names to a wheel that is going public — build_agent_worker_environment, IsolatedAgentProxy, agent_worker_internal_command — with no __all__, and src/coder_eval/isolation/__init__.py still declares __all__ = ["DockerRunner"], so orchestrator.py:1317 reaches past the package's curated surface (from coder_eval.isolation.agent_worker import IsolatedAgentProxy). Decide explicitly: either export IsolatedAgentProxy from the package __all__, or underscore-prefix build_agent_worker_environment to mark the module private.

What's Missing

Parallel paths:

• 🟠 The harness-env scrub was deleted wholesale but only re-implemented for the isolated worker. utils.scrub_agent_env_overrides() / AGENT_ENV_SCRUB_VARS were called UNCONDITIONALLY (not gated on isolation) by all three agents — claude_code_agent._build_sdk_env (base_env = scrub_agent_env_overrides()), codex_agent._build_codex_env (env = scrub_agent_env_overrides()), and antigravity's _harness_spawn_lock os.environ.pop window — and their replacement, agent_worker.build_agent_worker_environment(), is reachable ONLY from IsolatedAgentProxy._spawn (grep: 1 production call site). So host-driver runs, agent_isolation: false docker runs, and the agent_judge sub-agent now hand SKILLS_REPO_PATH, TASK_DIR, CODER_EVAL_* and AWS_BEARER_TOKEN_BEDROCK straight to the evaluated agent. The deleted comment justified the Bedrock entry specifically as preventing an inherited token from silently steering a DirectRoute run onto Bedrock (the CLI auto-selects on process.env.AWS_BEARER_TOKEN_BEDROCK) — that mis-routing risk is back for every non-containerized run, which is the common local/CI path. Neither docs/DOCKER_ISOLATION.md:288 ("Harness-only variables such as SKILLS_REPO_PATH, TASK_DIR, and CODER_EVAL_* are removed from agent SDK environments", still stated unconditionally) nor the two tests that covered it (rewritten in tests/test_docker_identity_isolation.py to assert only build_agent_worker_environment()) was updated to the narrower reality. (trigger: src/coder_eval/utils.py)
• 🟡 evaluation/sub_agent.py:202 is the one remaining agent-construction path that does NOT go through IsolatedAgentProxy — it instantiates ClaudeCodeAgent directly in the privileged process for agent_judge — and this PR removed its two protections in passing (cli_path=CONTAINER_CLAUDE_SHIM if agent_isolation_enabled() and the env scrub). The only thing keeping that safe is docker_runner._validate_agent_isolation_compatibility's criterion denylist, whose own comment frames it as temporary ("until they have a separate grader sandbox"): whoever lifts that denylist now silently gets a judge agent running as root with the evaluator's full environment, where before it would have dropped to UID 2000 through the shim. Either give SubAgentRunner the proxy/worker path too, or note in the denylist that it is now the sole guard. (trigger: src/coder_eval/agents/claude_code_agent.py)
• 🔵 The new doc claim (docs/DOCKER_ISOLATION.md:39, "applies equally to the built-in agents and to third-party AgentRegistry plugins … there is no built-in-kind allowlist") is not matched by the remaining per-kind staging in cli/run_task_internal_command.py:104-107, which still grant_agent_workspace(AGENT_HOME/.claude) for the claude-code state copy only. A third-party plugin agent that needs host-mounted state has no equivalent seam, so "kind-agnostic" holds for the launch boundary but not for state staging — say so, or generalize the grant. (trigger: docs/DOCKER_ISOLATION.md)
• 🔵 The same Dockerfile hunk that deleted the claude wrapper also deleted npm config set prefix /usr/local AND its verifying assertion test "$(command -v claude)" = "/usr/local/bin/claude" (docker/Dockerfile:52-61). The claude binary therefore moves to the nodesource default prefix with nothing left pinning or checking its location. Nothing in-tree references the old path so this is currently benign, but the two removals are unrelated to the worker refactor and the image now has one less build-time invariant; docker/Dockerfile.runtime still pins its own --prefix /opt/coder-eval/node, so the two images' claude locations are now derived by different mechanisms with no parity test (tests/test_image_from_dockerfiles.py only pins CLAUDE_CODE_VERSION). (trigger: docker/Dockerfile)

Tests:

• 🟡 Two whole features now cross the new boundary with zero coverage, and neither is in the set Axis 3 enumerates. (a) Early stop: the cooperative should_stop seam is re-implemented as a flag-file bridge (_new_stop_path() → stop_path param → worker's lambda: stop_path.exists() at agent_worker.py:254, published only from _publish_stop_flag_if_needed on event arrival), so every stop_early: arming — a documented, gate-affecting feature with its own EarlyStopWatcher test suite — runs through untested machinery under the default docker config; nothing asserts a watcher decision actually reaches the worker, nor how the added event-arrival latency interacts with decide_within's tool-call step counting. (b) Simulation/dialog mode reuses ONE worker across N communicate RPCs (agent state lives in _WorkerServer.agent); no test drives two turns against the same worker. (trigger: src/coder_eval/isolation/agent_worker.py)
• 🟡 CONTAINER_DROP_SHIM is now the SOLE privilege-drop seam (agent_worker.py:375 spawns through it; the second constant CONTAINER_CLAUDE_SHIM was deleted), yet no test pins that Python constant to docker/Dockerfile's COPY/chmod 0555 destination — grep finds only two tests that read the script's contents. The repo already establishes exactly this pattern for the other container path (tests/test_image_from_dockerfiles.py::test_container_entrypoint_matches_dockerfile_copy_destination, plus test_runtime_kit_entrypoint_matches_host_path). Renaming or relocating the shim in the Dockerfile would now surface only as a runtime spawn failure inside a container. The Dockerfile's new python -I -m coder_eval.isolation.agent_worker < /dev/null smoke covers the module path but not the shim path. (trigger: docker/Dockerfile)
• 🟡 The start payload silently assumes three separate JSON round-trips work for every registry kind — config.model_dump(mode="json") → registration.config_class.model_validate, dataclasses.asdict(route) → route_type(**data), and constructor_kwargs (LiteLLM cost_log_tags) — but tests/test_agent_worker.py exercises them only against a bare _PluginConfig with no fields and a hardcoded {"type": "DirectRoute", "data": {"judge_transport": None}} literal. A pytest.mark.parametrize over AgentRegistry kinds (claude-code / codex / antigravity / none) plus typing.get_args(ApiRoute) would cost nothing and would also close the route-codec exhaustiveness hole. (trigger: src/coder_eval/isolation/agent_worker.py) (restates: Axis 1: New worker RPC codec hand-mirrors the ApiRoute union, the error hierarchy and the Agent getters in literal maps, with no exhaustiveness/parity guard)
• 🟡 CI's only end-to-end exercise of the isolated worker is tasks/byod_smoke_test.yaml (tags smoke-pass, driver: docker, image FROM coder-eval-agent:latest so it inherits uid-gid-v1 and isolation defaults on) — a single "Do nothing" turn on the happy path. Every failure path stays unexercised at every level: smoke_task_timeout (the sleep 300 vs task_timeout: 30 task that exists precisely to regression-test the watchdog hard kill, i.e. the path where the isolated proxy now loses token_usage/messages/agent_output) runs on the HOST driver, and smoke-fail's smoke_budget_exceeded/smoke_negative_path likewise. Give one timeout/crash smoke task driver: docker, or the newly-degraded kill path ships with neither unit nor e2e coverage. (trigger: src/coder_eval/orchestrator.py) (restates: Axis 8: Hard-killed isolated turns lose token/cost/transcript telemetry)

Downstream consumers:

• 🟡 _raise_remote_error's 7-entry exception_types map has three downstream consumers that the PR does not consider: errors/categorization.py (typed checks run FIRST, before the string patterns), errors/retry.py, and the report/telemetry error_category rows. Its .get(error_type, AgentCrashError) default lands on ErrorCategory.AGENT_CRASH — the one agent category marked retryable ("CLI crashes are often transient") — so any unmapped worker-side type is not merely relabelled but re-classified from NOT-retryable to retryable, re-running a doomed turn at full LLM cost. _error_payload serializes type(exc).__name__ for any exception, and the map omits e.g. BudgetExceededError, EvaluationTimeoutError, TaskTimeoutError, JudgeInfrastructureError, plus the anthropic.RateLimitError/AuthenticationError classes _categorize_by_exception_type explicitly handles for the agent component (rate-limit's long backoff is lost; only the preserved message string still hints at the category). (trigger: src/coder_eval/isolation/agent_worker.py) (restates: Axis 1: New worker RPC codec hand-mirrors the ApiRoute union, the error hierarchy and the Agent getters in literal maps, with no exhaustiveness/parity guard)
• 🟡 The agent-authoring contract that third-party plugins (this PR's stated beneficiaries) are told to follow was not updated for the new process boundary. CLAUDE.md's "Adding a New Agent" steps 5-7 still describe an in-process agent, but an agent must now additionally: be constructible from a JSON-round-tripped config in a separate UID-2000 process, accept only JSON-serializable constructor_kwargs, keep its own sys.stdout clean (fd 1 is the RPC frame channel — an unframed print() from a plugin or its libraries is swallowed as a log line, and a mid-line interleave against _WorkerWriter's lock raises malformed agent-worker protocol line and hard-kills the turn), and expose everything the root side needs through the four snapshot keys, since only state/pending_turn/sdk_options/environment cross back. Note also that IsolatedAgentProxy.communicate itself does NOT call the mandated _begin_turn()/_end_turn_ok() (it mirrors state via _apply_snapshot) — a deliberate deviation worth writing down rather than leaving as a counter-example. (trigger: src/coder_eval/isolation/agent_worker.py)

Display & mapping dicts:

• 🟡 Nothing records that a run used the isolated worker. _record_route_environment_info (orchestrator.py:1206-1230) writes api_routing, eval_routing, aws_region, judge_transport, litellm_* and merges the agent's own get_environment_info(), but gains no agent_isolation / worker-protocol key — so no report, run.json row, or evalboard column distinguishes an isolated run from an in-process one. That is exactly the dimension a triager needs now that isolation changes what the artifacts contain (killed turns with token_usage=None, task.log missing agent DEBUG records, [agent-worker] stderr relays), and it is a one-line addition next to the route keys. (trigger: src/coder_eval/orchestrator.py)

Daily/nightly:

• 🟠 The PR states no blast radius for the production container path even though docker.agent_isolation defaults to true, so from merge onward EVERY containerized nightly task drives its agent through a brand-new 663-line RPC layer measured at 28.6% coverage. Three consequences need writing into the PR body: (1) the image is a lockstep prerequisite — enforcement moved from the host (_validate_agent_isolation_compatibility) into the image (IsolatedAgentProxy) while org.coder-eval.agent-isolation="uid-gid-v1" is unchanged and _preflight_image_version only warns, so the window between merging and docker-publish.yml pushing :latest is a real host/image skew window; (2) nightly report/cost consumers (the external eval-runner and evalboard dashboards) can now see under-reported total_token_usage/cost and empty transcripts on any task that hits task_timeout; (3) task.log's agent-side DEBUG layer is gone for these runs, which is the artifact the nightly triage flow starts from. (trigger: src/coder_eval/isolation/docker_runner.py) (restates: Axis 8: Host-side agent-kind allowlist removed without bumping the uid-gid-v1 capability label)

Harness & Lint Improvements

This section is long enough that it pushed the comment past GitHub's 65,536-character limit — it is posted in full as the follow-up comment below (12 proposed static checks incl. CE035–CE045, plus 7 harness improvements).

Top 5 Priority Actions

1. Preserve telemetry across hard kills: have IsolatedAgentProxy.kill_sync SIGTERM the worker (not the whole session) with a short grace so the in-worker agent's cancel handler emits the terminal AgentEndEvent before escalating to killpg(SIGKILL) — today the fallback at src/coder_eval/isolation/agent_worker.py:588 yields token_usage=None/agent_output="", so every isolated run that hits task_timeout under-reports cost and can change an llm_judge score for identical agent output; add a test asserting a killed isolated turn still carries non-None token_usage.
2. Make the isolation boundary fail closed on image skew: bump LABEL org.coder-eval.agent-isolation to uid-gid-v2 in docker/Dockerfile:110 and require the new value in _preflight_agent_isolation_image (src/coder_eval/isolation/docker_runner.py:288), because removing the host-side agent-kind allowlist at src/coder_eval/isolation/docker_runner.py:701 lets a stale-but-labeled image silently run a third-party plugin agent as root with full access to /opt/coder-eval/grader, and state the make docker-image/ghcr publish as a lockstep merge prerequisite.
3. Close the worker-hijack path that lets a graded agent fabricate its own trajectory: call prctl(PR_SET_DUMPABLE, 0) in agent_worker_internal_command() (src/coder_eval/isolation/agent_worker.py:331) before any agent is constructed — in a default container a same-UID sibling can read the RPC nonce from /proc/<worker>/mem and PTRACE_ATTACH, then emit frames the root harness trusts at src/coder_eval/isolation/agent_worker.py:416-440 to forge the TurnRecord that command_executed/skill_triggered/commands_efficiency grade and to suppress total_cost_usd past the max_usd gate.
4. Give _request a per-method deadline (src/coder_eval/isolation/agent_worker.py:484), since the ping handshake (line 390) and the start RPC run before the ThreadedWatchdog arms task_timeout at src/coder_eval/orchestrator.py:495 and no layer above puts a wall clock on them, so a live-but-mute worker — e.g. a third-party plugin blocking in ensure_plugins_loaded() at line 224 — hangs the task, the batch and the nightly indefinitely; also stop suppressing OSError on response writes (line 197) so the parent sees EOF instead of waiting on a future nothing resolves.
5. Build the missing behavioral tests for the proxy/worker boundary (src/coder_eval/isolation/agent_worker.py is 28.6% covered yet is now the production agent for every containerized run): extract the launch argv at lines 375-379 into an overridable seam so tests can drive the worker in-process against a registry test agent, then cover a normal turn round-trip, the crashed=True partial handoff (lines 587-593), TurnTimeoutError field preservation and both fail-closed gates, and replace the literal decode maps with derived ones — an ApiRoute round-trip case in tests/test_route_seam_exhaustiveness.py plus an SSOT exception map, so the five unlisted in-tree error types stop collapsing to AgentCrashError at line 520.

Stats: 0 🔴 · 4 🟠 · 6 🟡 · 6 🔵 across 8 axes reviewed.